Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

D3 external as script#71

Closed
gregmagolan wants to merge 3 commits intoangular:masterfrom
gregmagolan:d3-external
Closed

D3 external as script#71
gregmagolan wants to merge 3 commits intoangular:masterfrom
gregmagolan:d3-external

Conversation

@gregmagolan
Copy link
Contributor

@gregmagolan gregmagolan commented Jan 28, 2018

No description provided.

@gregmagolan gregmagolan changed the title WIP: d3 external D3 external as script Jan 29, 2018
Copy link
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems pretty good.

@evmar do you have a couple min to look at this? We are trying to replicate "the good part" of google3 setup

cmd = "cp $< $@",
)

genrule(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should clean this up - can be in a later PR. the ts_devserver should have an option that allows us to serve additional rootdirs, I think this is already present but maybe not wired up?

@@ -0,0 +1,3 @@
<div>Hello {{ name }}</div>
<input type="text" [value]="name" (input)="name = $event.target.value"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need an actual example app. I'll ask in the GDE channel to see if we can get something good

@@ -0,0 +1,5 @@
import * as _d3 from 'd3';

declare global {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we check this in, it needs a bunch of comments explaining what this is, how to use it, how to formulate one for a different library, etc. - since this is the canonical example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add some comments


load("@build_bazel_rules_typescript//:defs.bzl", "ts_library")

ts_library(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this merits a whole separate bazel package - why not put the shim in something like typings.d.ts in the src/ directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work. I like having them in their own folder but a single typings.d.ts file will do the trick.

// any ts_library or ng_module that requires these types and it makes them available
// as global variables that are properly typed in typescript.
declare global {
const d3: typeof _d3;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is the opposite of what we want, long-term -- we want users to import d3 if they use it, so that taze can add the proper dependency when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex and I both agree that we need another example with d3 imported properly. This example is the only way that works at the moment with where ts_devserver and the rollup_bundle production bundler is at right now. Getting it to work with an import will require changes to both and needs to some thought.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evmar my understanding is that some libraries are documented such that developers expect to import them, but some are only accessible via globals. I think ABC needs to demonstrate both ways? If we only document how to use ES imports then I think some libraries would be unusable?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the typings usually match how the library works, so you only need a shim thing like here when you're using the library in a way contrary to its intended use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregmagolan I'm hesitant to merge this if the intended use of d3 is to import it.
What's a library that provides a better test case for ambient imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of one off the top of my head. I'll think about it.

@alexeagle
Copy link
Contributor

I guess this is dead, and we'll use ngrx as the third-party library example

@alexeagle alexeagle closed this May 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants